Code Review: Tab-Based PKCE Authentication (Phase 4)

I've reviewed the implementation and found 3 critical bugs and 2 high-priority issues that should be addressed before merging.

🔴 Critical Bug #1: Memory Leak - removeListener Never Cleaned Up

File: src/auth/TabBasedPKCEAuth.ts lines 319-326, 178-195

The removeListener callback added to browser.tabs.onRemoved at line 326 is never removed when authentication succeeds.

// Line 326: Listener is added
const removeListener = (tabId: number) => { ... };
browser.tabs.onRemoved.addListener(removeListener);

// Line 178-195: cleanup() removes tabListener but NOT removeListener
const cleanup = async () => {
  if (tabListener) {
    browser.tabs.onUpdated.removeListener(tabListener);  // ✅ Good
    tabListener = undefined;
  }
  // ❌ MISSING: browser.tabs.onRemoved.removeListener(removeListener);
};

Impact: Every successful auth leaves a dangling listener that continues to fire on all future tab closures.

Fix: Add removeListener to the cleanup() function or track it as a variable like tabListener.

🔴 Critical Bug #2: Race Condition - Listener Registered Before authTabId Set

File: src/auth/TabBasedPKCEAuth.ts lines 319-331

// Line 326: Listener added HERE - authTabId is still undefined
browser.tabs.onRemoved.addListener(removeListener);

// Lines 329-331: Tab created and authTabId assigned HERE - AFTER listener
const tab = await browser.tabs.create({ url: authUrl, active: true });
authTabId = tab.id;

Issue: Between lines 326-331, if any tab is closed (not related to auth), removeListener executes with authTabId === undefined. Since the check tabId === authTabId will fail (comparing to undefined), it passes without cleanup. However, the listener remains forever active.

Fix: Move addListener after authTabId is assigned, or add a guard: if (authTabId === undefined) return;

🔴 Critical Bug #3: cleanup() Not Awaited in Synchronous Callbacks

File: src/auth/TabBasedPKCEAuth.ts lines 236-243, 247-254, 294-302, 322-324

The tabListener and removeListener are synchronous callbacks, but they call cleanup() (an async function) without awaiting:

// Lines 236-243: cleanup() not awaited
cleanup();         // ❌ Returns immediately
clearPKCEState();  // ❌ Races with cleanup()
resolve({...});    // ❌ Resolves before tab is actually closed

Impact: The promise resolves before browser.tabs.remove() completes, and clearPKCEState() may race with storage operations inside cleanup().

Fix: Either make cleanup synchronous (remove the await inside) or restructure to handle async properly. Consider using a flag pattern like JwtManager.ts uses with isClearing.

🟡 High Priority: Concurrent Auth Call Vulnerability

File: src/auth/TabBasedPKCEAuth.ts lines 161-356

If authenticateWithTabBasedPKCE() is called multiple times concurrently (e.g., user rapid-clicks "Sign In"), each call registers its own listeners. Only the first call's cleanup will run, leaving others orphaned.

Fix: Add a guard flag at module scope to prevent concurrent auth attempts:

let authInProgress = false;

export async function authenticateWithTabBasedPKCE(): Promise<OAuthResult> {
  if (authInProgress) {
    return { success: false, error: 'auth_in_progress', errorDescription: 'Authentication already in progress' };
  }
  authInProgress = true;
  try {
    // ... existing code
  } finally {
    authInProgress = false;
  }
}

🟡 High Priority: removeListener Self-Removes But May Leave Artifacts

File: src/auth/TabBasedPKCEAuth.ts lines 319-325

const removeListener = (tabId: number) => {
  if (tabId === authTabId) {
    browser.tabs.onRemoved.removeListener(removeListener);  // Self-removes
    cleanup();
    clearPKCEState();
    // ❌ But never resolves the authPromise - leaves it hanging on timeout
  }
};

When user closes the auth tab, the removeListener fires and cleans up, but the authPromise is never resolved/rejected directly. It will eventually timeout after 5 minutes.

Fix: Add resolve() call in removeListener with appropriate error response.

✅ What Looks Good

• PKCE flow logic is correct
• State validation and CSRF protection are properly implemented
• Token exchange follows OAuth 2.1 spec
• Test coverage is comprehensive (14 tests)
• User agent detection functions are well-implemented

Summary

🤖 Generated with Claude Code

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}